feat(minor): Disable jemalloc using package traits#347
feat(minor): Disable jemalloc using package traits#347JaapWijnen wants to merge 6 commits intoordo-one:mainfrom
Conversation
|
This PR title should have been marked with feat: I can't seem to edit the PR title however. @hassila |
|
Thanks @JaapWijnen - but we will soon remove the jemalloc dependency completely through #333 - although that will require a 6.3 toolchain - so maybe worth landing this for 6.0-6.2 support regardless - @JaapWijnen what is you earliest supported toolchain at this point? @supersonicbyte over to you |
I agree this is probably worth landing for 6.0 - 6.2. Will do an extensive PR review soon. |
|
Oh that's great! |
|
Yeah, when 6.3 lands we will bump this package to 6.1-6.2 for the old jemalloc and 6.3 for the new malloc interposer (that requires bug fixes from SPM that only exist in 6.3...) |
|
Ah I see. Awesome. Yeah we'll be moving to 6.3 as soon as it lands so that's great to hear :) Thanks! |
supersonicbyte
changed the title
|
@JaapWijnen everything looks good! just a quick comment. In order to fix the build for linux we should add #if canImport(Darwin)
@preconcurrency import Darwin
#elseif canImport(Glibc)
@preconcurrency import Glibc
#elseif canImport(Musl)
@preconcurrency import Musl
#else
#error("Unsupported Platform")
#endifRegarding the crash when running It's a bug that happens when xctest runs with jemalloc. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| .target( | ||
| name: "Benchmark", | ||
| dependencies: [ | ||
| .product(name: "Histogram", package: "hdrhistogram-swift"), | ||
| .product(name: "ArgumentParser", package: "swift-argument-parser"), | ||
| .product(name: "SystemPackage", package: "swift-system"), | ||
| .byNameItem(name: "CDarwinOperatingSystemStats", condition: .when(platforms: [.macOS, .iOS])), | ||
| .byNameItem(name: "CLinuxOperatingSystemStats", condition: .when(platforms: [.linux])), | ||
| .product(name: "Atomics", package: "swift-atomics"), | ||
| "SwiftRuntimeHooks", | ||
| "BenchmarkShared", | ||
| .product(name: "jemalloc", package: "package-jemalloc", condition: .when(platforms: [.macOS, .linux], traits: ["EnableJemalloc"])), | ||
| ], | ||
| swiftSettings: [.swiftLanguageMode(.v5)] | ||
| ), |
There was a problem hiding this comment.
Critical Bug: Missing compilation condition definition for EnableJemalloc trait
The code uses #if EnableJemalloc in source files (MallocStatsProducer+jemalloc.swift and OperatingSystemAndMallocTests.swift), but the Benchmark target doesn't define EnableJemalloc as a compilation condition. Package traits only control dependency resolution - they don't automatically become preprocessor flags.
This causes:
- The jemalloc dependency gets linked when the trait is enabled
- But the code using jemalloc is never compiled (the
#if EnableJemallocblocks are always skipped) - This creates a broken state where the dependency exists but is unused
Fix: Add the compilation condition to swiftSettings:
.target(
name: "Benchmark",
dependencies: [...],
swiftSettings: [
.swiftLanguageMode(.v5),
.define("EnableJemalloc", .when(traits: ["EnableJemalloc"]))
]
)The same fix should be applied to BenchmarkTests target (lines 112-131) which also likely needs this compilation condition for its test code.
| .target( | |
| name: "Benchmark", | |
| dependencies: [ | |
| .product(name: "Histogram", package: "hdrhistogram-swift"), | |
| .product(name: "ArgumentParser", package: "swift-argument-parser"), | |
| .product(name: "SystemPackage", package: "swift-system"), | |
| .byNameItem(name: "CDarwinOperatingSystemStats", condition: .when(platforms: [.macOS, .iOS])), | |
| .byNameItem(name: "CLinuxOperatingSystemStats", condition: .when(platforms: [.linux])), | |
| .product(name: "Atomics", package: "swift-atomics"), | |
| "SwiftRuntimeHooks", | |
| "BenchmarkShared", | |
| .product(name: "jemalloc", package: "package-jemalloc", condition: .when(platforms: [.macOS, .linux], traits: ["EnableJemalloc"])), | |
| ], | |
| swiftSettings: [.swiftLanguageMode(.v5)] | |
| ), | |
| .target( | |
| name: "Benchmark", | |
| dependencies: [ | |
| .product(name: "Histogram", package: "hdrhistogram-swift"), | |
| .product(name: "ArgumentParser", package: "swift-argument-parser"), | |
| .product(name: "SystemPackage", package: "swift-system"), | |
| .byNameItem(name: "CDarwinOperatingSystemStats", condition: .when(platforms: [.macOS, .iOS])), | |
| .byNameItem(name: "CLinuxOperatingSystemStats", condition: .when(platforms: [.linux])), | |
| .product(name: "Atomics", package: "swift-atomics"), | |
| "SwiftRuntimeHooks", | |
| "BenchmarkShared", | |
| .product(name: "jemalloc", package: "package-jemalloc", condition: .when(platforms: [.macOS, .linux], traits: ["EnableJemalloc"])), | |
| ], | |
| swiftSettings: [ | |
| .swiftLanguageMode(.v5), | |
| .define("EnableJemalloc", .when(traits: ["EnableJemalloc"])) | |
| ] | |
| ), |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
@supersonicbyte Done! |
|
@JaapWijnen thanks! I removed the One more thing, @hassila actually noticed that the trait In order to preserve backward compatibility could you flip the logic so:
Additionally, we can also rename the trait to Sorry for the additional comment, I should have seen this in the first review pass. |
|
Good point! All done 👍 |
targets Benchmark and BenchmarkTool have to be set to language mode v5 due to concurrency errors.
# Conflicts: # Package.resolved
JaapWijnen
force-pushed
the
disable-jemalloc-using-package-traits
branch
from
3b9d577 to
2566488
Compare
3 participants
Description
This is a first attempt at enabling/disable jemalloc using package traits #316
This shows off what a trait based implementation could look like. But I'm not exactly sure what the expected behaviour would be on unsupported platforms. So that probably needs some more discussion.
How Has This Been Tested?
I performed a build and ran the tests with and without the trait enabled.
The build succeeds on Mac but running tests (with the trait enabled) fails with a segmentation error.
I'm not entirely sure where this is coming from but seems to fail once we add
package-jemallocto the package dependencies. Regardless of if it is actually used by package-benchmark this seems to crash runningswift test --traits EnableJemallocThis user experience should be improved ofcourse. Which I'm hoping to discuss in the PR.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package